Skip to content

Conversation

@dermatz
Copy link
Collaborator

@dermatz dermatz commented Jan 12, 2026

No description provided.

dermatz and others added 18 commits January 10, 2026 00:52
- New command: mageforge:hyva:compatibility:check (aliases: m:h:c:c, hyva:check)
- Scans Magento modules for Hyvä theme incompatibilities
- Detects RequireJS, Knockout.js, jQuery, UI Components usage
- Options: --show-all, --third-party-only, --include-vendor, --detailed
- Service layer: CompatibilityChecker, ModuleScanner, IncompatibilityDetector
- Exit code 1 for critical issues, 0 for success
- Updated CI/CD workflow and documentation
- Changed default behavior from 0 modules to third-party modules (18)
- Without flags: Scans third-party only (excludes Magento_*)
- With --include-vendor: Scans all 394 modules including Magento core
- Updated documentation to reflect new default behavior
- Interactive mode activated when no options provided
- Multi-select menu for scan options:
  * Show all modules
  * Include Magento core modules
  * Detailed file-level issues
- Displays selected configuration before scan
- Falls back to direct mode if interactive fails
- Consistent with theme:build command UX
- New option: Show only incompatible modules (pre-selected by default)
- Improves UX by filtering out compatible modules
- Users can still choose 'Show all' for complete overview
- Configuration display updated to reflect selection
* Initial plan

* Add actual execution tests for Hyvä compatibility checker command

Co-authored-by: dermatz <[email protected]>

---------

Co-authored-by: copilot-swe-agent[bot] <[email protected]>
Co-authored-by: dermatz <[email protected]>
Co-authored-by: Mathias Elle <[email protected]>
#61)

* Initial plan

* Clarify exit code documentation for Hyvä compatibility checker

Co-authored-by: dermatz <[email protected]>

---------

Co-authored-by: copilot-swe-agent[bot] <[email protected]>
Co-authored-by: dermatz <[email protected]>
Co-authored-by: Mathias Elle <[email protected]>
Co-authored-by: copilot-swe-agent[bot] <[email protected]>
Co-authored-by: Mathias Elle <[email protected]>
…terns, enhance security (#65)

* Initial plan

* Apply code review feedback: fix duplicate di.xml, improve regex patterns, safer environment handling

Co-authored-by: dermatz <[email protected]>

* Replace error suppression with explicit exception handling in TTY detection

Co-authored-by: dermatz <[email protected]>

* Fix RequireJS pattern regex to properly match module references

Co-authored-by: dermatz <[email protected]>

---------

Co-authored-by: copilot-swe-agent[bot] <[email protected]>
Co-authored-by: dermatz <[email protected]>
…egex patterns, enhance security (#65)"

This reverts commit f77c7ac.
Copilot AI review requested due to automatic review settings January 12, 2026 10:02
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces a Hyvä theme compatibility checker feature that scans Magento modules for potential incompatibilities with Hyvä themes. The checker detects problematic patterns like RequireJS, Knockout.js, jQuery, and UI Components usage.

Changes:

  • Added new mageforge:hyva:compatibility:check command with interactive mode support and multiple CLI options
  • Implemented three new service classes for scanning, detection, and compatibility checking
  • Updated documentation with comprehensive command details and pattern descriptions

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 14 comments.

Show a summary per file
File Description
src/etc/di.xml Registers new Hyvä compatibility command and updates DI configuration (contains critical duplicate type definition)
src/Service/Hyva/ModuleScanner.php New service for recursively scanning module directories and detecting Hyvä compatibility packages
src/Service/Hyva/IncompatibilityDetector.php New service implementing pattern matching for detecting RequireJS, Knockout, jQuery, and UI Component usage
src/Service/Hyva/CompatibilityChecker.php New orchestrator service that coordinates scanning, filtering, and result formatting
src/Console/Command/Hyva/CompatibilityCheckCommand.php New CLI command with interactive mode, multiple options, and detailed reporting capabilities
docs/commands.md Comprehensive documentation for the new command including usage examples, options, and detected patterns
CHANGELOG.md Documents the new feature with bullet points describing capabilities
.github/workflows/magento-compatibility.yml Adds CI/CD tests for the new command across multiple Magento versions

Comment on lines 6 to 38
<type name="Magento\Framework\Console\CommandList">
<arguments>
<argument
name="commands"
xsi:type="array"
>
<item name="mageforge_system_version"
xsi:type="object"
>
OpenForgeProject\MageForge\Console\Command\System\VersionCommand</item>
<item name="mageforge_system_check"
xsi:type="object"
>
OpenForgeProject\MageForge\Console\Command\System\CheckCommand</item>
<item name="mageforge_theme_list"
xsi:type="object"
>
OpenForgeProject\MageForge\Console\Command\Theme\ListCommand</item>
<item name="mageforge_theme_build"
xsi:type="object"
>
OpenForgeProject\MageForge\Console\Command\Theme\BuildCommand</item>
<item name="mageforge_theme_watch"
xsi:type="object"
>
OpenForgeProject\MageForge\Console\Command\Theme\WatchCommand</item>
<item name="mageforge_hyva_compatibility_check"
xsi:type="object"
>
OpenForgeProject\MageForge\Console\Command\Hyva\CompatibilityCheckCommand</item>
</argument>
</arguments>
</type>
Copy link

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicate type definition detected. There are two identical type definitions for "Magento\Framework\Console\CommandList" (lines 6-38 and lines 39-65). This will cause a configuration conflict in Magento's dependency injection system. The command registrations from lines 6-38 should be merged into the existing type definition at lines 39-65, or the duplicate definition should be removed entirely.

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot open a new pull request to apply changes based on this feedback

Comment on lines 6 to 38
<type name="Magento\Framework\Console\CommandList">
<arguments>
<argument
name="commands"
xsi:type="array"
>
<item name="mageforge_system_version"
xsi:type="object"
>
OpenForgeProject\MageForge\Console\Command\System\VersionCommand</item>
<item name="mageforge_system_check"
xsi:type="object"
>
OpenForgeProject\MageForge\Console\Command\System\CheckCommand</item>
<item name="mageforge_theme_list"
xsi:type="object"
>
OpenForgeProject\MageForge\Console\Command\Theme\ListCommand</item>
<item name="mageforge_theme_build"
xsi:type="object"
>
OpenForgeProject\MageForge\Console\Command\Theme\BuildCommand</item>
<item name="mageforge_theme_watch"
xsi:type="object"
>
OpenForgeProject\MageForge\Console\Command\Theme\WatchCommand</item>
<item name="mageforge_hyva_compatibility_check"
xsi:type="object"
>
OpenForgeProject\MageForge\Console\Command\Hyva\CompatibilityCheckCommand</item>
</argument>
</arguments>
</type>
Copy link

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent indentation style detected. Lines 6-38 use 6 spaces for indentation while lines 39-88 use 4 spaces. According to Magento Coding Standards and the project guidelines, indentation should be consistent throughout the file using 4 spaces. The first type definition (lines 6-38) should be updated to use 4-space indentation to match the rest of the file.

Copilot uses AI. Check for mistakes.
src/etc/di.xml Outdated
Comment on lines 32 to 35
<item name="mageforge_hyva_compatibility_check"
xsi:type="object"
>
OpenForgeProject\MageForge\Console\Command\Hyva\CompatibilityCheckCommand</item>
Copy link

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new hyva compatibility check command is registered in the duplicate type definition (line 32-35) but missing from the existing type definition (lines 39-65) which includes the static clean command. When the duplicate is removed, this command registration should be added to the remaining type definition to ensure it's properly registered.

Copilot uses AI. Check for mistakes.
Comment on lines 167 to 174
* Get severity color for console output
*/
public function getSeverityColor(string $severity): string
{
return match ($severity) {
self::SEVERITY_CRITICAL => 'red',
self::SEVERITY_WARNING => 'yellow',
default => 'white',
Copy link

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method getSeverityColor returns color names as strings ('red', 'yellow', 'white'), but these are Symfony Console color names. The method should be documented to clarify that these are Symfony Console color tags. Additionally, the 'white' default color may not be visible on light terminal backgrounds. Consider using a more neutral default like 'gray' or documenting terminal compatibility considerations.

Suggested change
* Get severity color for console output
*/
public function getSeverityColor(string $severity): string
{
return match ($severity) {
self::SEVERITY_CRITICAL => 'red',
self::SEVERITY_WARNING => 'yellow',
default => 'white',
* Get severity color for console output.
*
* The returned string is a Symfony Console color/style name that can be
* used in console formatting tags (e.g. <fg=red> or style definitions).
*
* - critical => "red"
* - warning => "yellow"
* - default => "gray" (chosen to remain visible on light terminals)
*/
public function getSeverityColor(string $severity): string
{
return match ($severity) {
self::SEVERITY_CRITICAL => 'red',
self::SEVERITY_WARNING => 'yellow',
default => 'gray',

Copilot uses AI. Check for mistakes.
Comment on lines 114 to 164
private function isHyvaCompatibilityPackage(array $composerData): bool
{
// Check if this IS a Hyvä compatibility package
$packageName = $composerData['name'] ?? '';
if (str_starts_with($packageName, 'hyva-themes/') && str_contains($packageName, '-compat')) {
return true;
}

// Check dependencies for Hyvä packages
$requires = $composerData['require'] ?? [];
foreach ($requires as $package => $version) {
if (str_starts_with($package, 'hyva-themes/')) {
return true;
}
}

return false;
}

/**
* Check if module has Hyvä compatibility package (public wrapper)
*/
public function hasHyvaCompatibilityPackage(string $modulePath): bool
{
$composerPath = $modulePath . '/composer.json';

if (!$this->fileDriver->isExists($composerPath)) {
return false;
}

try {
$content = $this->fileDriver->fileGetContents($composerPath);
$composerData = json_decode($content, true);

if (!is_array($composerData)) {
return false;
}

return $this->isHyvaCompatibilityPackage($composerData);
} catch (\Exception $e) {
// Log error when debugging to help identify JSON or file read issues
if (getenv('MAGEFORGE_DEBUG')) {
error_log(sprintf(
'[MageForge][ModuleScanner] Failed to read composer.json at "%s": %s',
$composerPath,
$e->getMessage()
));
}
return false;
}
}
Copy link

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method isHyvaCompatibilityPackage is defined as private but has a public wrapper hasHyvaCompatibilityPackage that simply calls it. This adds unnecessary indirection. Consider either making isHyvaCompatibilityPackage public directly and removing the wrapper, or consolidating the logic. Additionally, the public wrapper has inconsistent error handling - it catches exceptions but the private method doesn't document that it may throw exceptions.

Copilot uses AI. Check for mistakes.
Comment on lines 95 to 103
} catch (\Exception $e) {
// Skip directories that can't be read, but log details when debugging
if (getenv('MAGEFORGE_DEBUG')) {
error_log(sprintf(
'[MageForge][ModuleScanner] Failed to read directory "%s": %s',
$directory,
$e->getMessage()
));
}
Copy link

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error logging uses getenv('MAGEFORGE_DEBUG') directly which is less testable and doesn't follow Magento best practices. Consider using Magento's configuration system or injecting a logger with appropriate log levels instead of custom environment variable checks. This would provide better integration with Magento's logging infrastructure.

Copilot uses AI. Check for mistakes.
}

// Display results
$this->displayResults($results, $displayShowAll || $detailed);
Copy link

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent display logic at lines 196-202. The variable 'displayShowAll' is set based on incompatibleOnly flag, but then at line 205 it's combined with $detailed flag using OR operator. This means if --detailed is used, all modules will be shown regardless of the incompatibleOnly setting, which may not be the intended behavior. The logic should clearly separate filtering (what to scan) from display (what to show in results).

Suggested change
$this->displayResults($results, $displayShowAll || $detailed);
$this->displayResults($results, $displayShowAll);

Copilot uses AI. Check for mistakes.

- `--show-all` / `-a` - Show all modules including compatible ones
- `--third-party-only` / `-t` - Check only third-party modules (exclude Magento\_\* modules)
- `--include-vendor` - Include Magento core modules in scan (default: third-party only)
Copy link

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documentation inconsistency: The description states "Include Magento core modules (default: third-party only)" at line 209, but looking at the actual command option definition and implementation, the --include-vendor flag controls whether to include vendor directory modules, not specifically Magento core modules. The Magento_* core modules are filtered by the --third-party-only flag. This distinction should be clarified in the documentation to accurately reflect the command's behavior.

Suggested change
- `--include-vendor` - Include Magento core modules in scan (default: third-party only)
- `--include-vendor` - Include modules from the `vendor/` directory in the scan (by default only non-vendor modules are scanned; Magento\_\* core modules are still controlled by `--third-party-only`)

Copilot uses AI. Check for mistakes.
**Options**:

- `--show-all` / `-a` - Show all modules including compatible ones
- `--third-party-only` / `-t` - Check only third-party modules (exclude Magento\_\* modules)
Copy link

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spelling inconsistency: "Magento_*" at line 208 uses an escaped underscore with backslash, which is correct for some Markdown renderers, but the same pattern at line 230 uses "Magento_*" without escaping. For consistency, both instances should use the same escaping pattern. Most modern Markdown renderers don't require escaping underscores when they're part of literal text, so consider removing the backslash escape for consistency.

Suggested change
- `--third-party-only` / `-t` - Check only third-party modules (exclude Magento\_\* modules)
- `--third-party-only` / `-t` - Check only third-party modules (exclude Magento_* modules)

Copilot uses AI. Check for mistakes.
Copy link
Contributor

Copilot AI commented Jan 12, 2026

@dermatz I've opened a new pull request, #67, to work on those changes. Once the pull request is ready, I'll request review from you.

@dermatz dermatz merged commit 8d92503 into main Jan 12, 2026
8 checks passed
@dermatz dermatz deleted the feature-hyva-compat-checker branch January 12, 2026 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants